Conversation
| return p, err | ||
| } | ||
|
|
||
| // This is a safeguard in case the validator function does not return a |
There was a problem hiding this comment.
it is more informative BUT it halts the entity application regular execution flow. As I see, it is up to the caller to inform "loudly" on errors which are consider critical for the regular execution of its business logic. Making a decision within a library which lacks on the context of its usage, can become highly disturbing for its usage
There was a problem hiding this comment.
I agree. Instead of returning a panic, I will return an error. It seems like changing the default behaviour from a panic to returning an error is the best option to let the caller know that the priority is incorrect in a more graceful way.
| // of `[0, priorities)` will panic. | ||
| func NewAdaptiveThrottle(priorities int, options ...AdaptiveThrottleOption) *AdaptiveThrottle { | ||
| if priorities <= 0 { | ||
| panic("bulwark: priorities must be greater than 0") |
There was a problem hiding this comment.
No a big fan of panics. I understand the goal is to make this change backward compatible but my concern isin certain cases where NewAdaptiveThrottle() is called depending on application states (no the case atm anywhere though) it might lead to unexpected service disruptions
2 participants
Work in progress to show how Bulwark could be configured to handle invalid priority values. All the options provide a different way to alert the developer that the library is misused.
The default behaviour went from panicking to a quiet adjustment with a warning log.